Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: upgrade golangci version and lint fixes #3443

Merged
merged 3 commits into from
Jun 6, 2023

Conversation

lvrach
Copy link
Member

@lvrach lvrach commented Jun 5, 2023

Description

I have upgraded golangci to version: v1.52, I have only specified up to minor versions so that patch upgrades will happen automatically.

With the upgrade, the following linters were reenabled again (disabled before due to generics)

Linting fixes

rowserrcheck: After rows.Next() iteration we need to check for rows.Err().
wastedassign: Assign and value and not use it. In most cases x := 0 -> var x int

Notion Ticket

https://www.notion.so/rudderstacks/Upgrade-golangci-version-lint-fixes-7abe589fec944d16b16ca378363ac8d1?pvs=4

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@lvrach lvrach changed the title Chore/golangci version chore: upgrade golangci version Jun 5, 2023
@lvrach lvrach changed the title chore: upgrade golangci version chore: upgrade golangci version and lint fixes Jun 5, 2023
@@ -12,11 +12,10 @@ Ping returns health check for pg database
func (jd *HandleT) Ping() error {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
rows, err := jd.dbHandle.QueryContext(ctx, `SELECT 'Rudder DB Health Check'::text as message`)
_, err := jd.dbHandle.ExecContext(ctx, `SELECT 'Rudder DB Health Check'::text as message`)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to call QueryContext if we are not interested in result

jobsdb/jobsdb.go Outdated Show resolved Hide resolved
@@ -273,7 +273,7 @@ func (t *Stats) GetRouterPickupJobs(destType string, noOfWorkers int, routerTime
break
}

pickUpCount := 0
var pickUpCount int
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linter was complaining: variable defined but got overwritten before used.

@@ -905,6 +905,8 @@ func (d *Deltalake) partitionQuery(ctx context.Context, tableName string) (strin
}
defer func() { _ = rows.Close() }()

_ = rows.Err() // ignore error
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to read Columns, but not the rows, no need to check for errors.

I prefer using _ = rows.Err(), instead of //nolint:rowserrcheck because it works across all linters.

@lvrach lvrach marked this pull request as ready for review June 5, 2023 11:54
@lvrach lvrach requested review from a team, achettyiitr and Sidddddarth and removed request for a team June 5, 2023 11:54
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Patch coverage: 10.47% and project coverage change: -0.07 ⚠️

Comparison is base (b70c075) 68.58% compared to head (85b6945) 68.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3443      +/-   ##
==========================================
- Coverage   68.58%   68.51%   -0.07%     
==========================================
  Files         330      330              
  Lines       52820    52905      +85     
==========================================
+ Hits        36224    36248      +24     
- Misses      14267    14308      +41     
- Partials     2329     2349      +20     
Impacted Files Coverage Δ
jobsdb/backup.go 75.00% <0.00%> (-0.41%) ⬇️
jobsdb/jobsdb_utils.go 92.37% <0.00%> (-2.41%) ⬇️
jobsdb/migration.go 76.98% <0.00%> (-0.60%) ⬇️
services/pgnotifier/pgnotifier.go 70.81% <0.00%> (-0.27%) ⬇️
services/validators/envValidator.go 50.45% <0.00%> (-0.71%) ⬇️
utils/misc/dbutils.go 33.33% <0.00%> (+1.41%) ⬆️
utils/misc/misc.go 53.17% <0.00%> (-0.18%) ⬇️
warehouse/api.go 68.76% <0.00%> (-0.70%) ⬇️
warehouse/client/client.go 46.73% <0.00%> (-1.58%) ⬇️
...ehouse/integrations/azure-synapse/azure-synapse.go 62.45% <0.00%> (-0.21%) ⬇️
... and 13 more

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@achettyiitr
Copy link
Member

achettyiitr commented Jun 5, 2023

I believe we introduced a couple of panics while handling the errors for the warehouse module. But I think we can take it as a separate PR to handle it.

@lvrach
Copy link
Member Author

lvrach commented Jun 6, 2023

I believe we introduced a couple of panics while handling the errors for the warehouse module. But I think we can take it as a separate PR to handle it.

My strategy was to mimic the way errors were handled in the function. Otherways the scope of PR could have been much bigger.

@cisse21 cisse21 merged commit 3d03653 into master Jun 6, 2023
@cisse21 cisse21 deleted the chore/golangci-version branch June 6, 2023 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants